[REVIEW NOTIFICATION]

This pull request has been approved by:

• JmPotato
• ngaut

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

@iosmanthus: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

/wip
investigating some test errors.

Investigated the CI error and found that the failing test was introduced by the previous client errors feature (#22382). In that PR, errors.As was used to check the type of the error.

Unfortunately, previous versions of pingcap/errors did not support the Unwrap function required by std errors.{Is, As} until pingcap/errors#35.

This PR happens to have upgraded pingcap/errors, which will cause cases that previously failed with errors.As conversion to now succeed.

It looks like extracting errors.Error from the wrap-over errors is the result we want. So I think we should change the expect result of test here, please help to confirm /cc @morgo

Investigated the CI error and found that the failing test was introduced by the previous client errors feature (#22382). In that PR, errors.As was used to check the type of the error.

Yes, specifically it is only used by this code:

		if cc.ctx != nil && cc.ctx.WarningCount() > 0 {
			for _, err := range cc.ctx.GetWarnings() {
				var warn *errors.Error
				if ok := goerr.As(err.Err, &warn); ok {
					code := uint16(warn.Code())
					errno.IncrementWarning(code, cc.user, cc.peerHost)
				}
			}
		}

It only needs to extract the code as a uint16 from each warning. I will confess I don't understand the error code types that well. If you can simplify this code, it will still work fine.

Investigated the CI error and found that the failing test was introduced by the previous client errors feature (#22382). In that PR, errors.As was used to check the type of the error.

Yes, specifically it is only used by this code:

		if cc.ctx != nil && cc.ctx.WarningCount() > 0 {
			for _, err := range cc.ctx.GetWarnings() {
				var warn *errors.Error
				if ok := goerr.As(err.Err, &warn); ok {
					code := uint16(warn.Code())
					errno.IncrementWarning(code, cc.user, cc.peerHost)
				}
			}
		}

It only needs to extract the code as a uint16 from each warning. I will confess I don't understand the error code types that well. If you can simplify this code, it will still work fine.

I didn't mean to change this code here, but upgrading of pingcap/errors caused errors.As to behave differently than before. This means that some scenarios where errors.Error could not be extracted before, can now be extracted via errors.As. I'm wonder if this change is acceptable.

I didn't mean to change this code here, but upgrading of pingcap/errors caused errors.As to behave differently than before. This means that some scenarios where errors.Error could not be extracted before, can now be extracted via errors.As. I'm wonder if this change is acceptable.

Yes, this is an improvement! If errors could not be extracted before it would mean that some warnings would not publish to the infoschema table. Having them there now is great (it's actually a bug fix; the code might not have tested all scenarios correctly.)

I didn't mean to change this code here, but upgrading of pingcap/errors caused errors.As to behave differently than before. This means that some scenarios where errors.Error could not be extracted before, can now be extracted via errors.As. I'm wonder if this change is acceptable.

Yes, this is an improvement! If errors could not be extracted before it would mean that some warnings would not publish to the infoschema table. Having them there now is great (it's actually a bug fix; the code might not have tested all scenarios correctly.)

Thanks for your reply. I'll update the tests.

/run-check_dev_2

/merge

This pull request has been accepted and is ready to merge.